-
Notifications
You must be signed in to change notification settings - Fork 8
Podman support and option to backup across all projects #66
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
lawndoc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice simplification of the container filter loop. Please see my comment about the shared network stack
This is fine, using socket should be backward compatible |
|
Doing some testing of this PR now, sorry it's taken me so long to get to this. Everything is going well in my previous test setup. Looks like the container filter changes will also fix #62 which is a nice bonus. Still need to test:
|
|
@AlexMcDermott your code is working as expected for individual projects, but it fails when backing up databases in other compose projects. This is because docker compose will create different networks for each compose project. You can import other networks into your compose project, but that's not a good user experience. Instead, I think we can give the backup container access to every network that it will need access to, instead of just the network defined for the main backup container. I actually like this better anyways, because then we don't have to mess with the backup container's network even within a single compose project. |
|
Hi @lawndoc thanks for the review. I've implemented your feedback and tested it out with a Postgres container started in another compose stack. Let me know what you think : ) |
|
Great work, looks like the right approach now. Looks like tests are failing. I don't have time to look closely right now but at a glance it looks like the mocks maybe aren't generating a valid network config for the |
|
Cheers, thanks for checking it out. Now I know its looking decent I'll fix up the tests |
Should be working now, added network settings to the fixture |
lawndoc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks for the contribution!
|
I believe there was a regression Here are my tests, making a backup using podman: v1.5.1: working |
|
You're right, I totally forgot what the original issue was for this PR when I fixed socket proxy issue in 1.5.4 I'll create a new issue and fix the regression with the approach from this PR. I'll look into adding an integration test for podman as well. |
Hi, cool project
Contents
Podman Hostname issue
I was running into this issue and found it more reliable to use the hostname from the socket library instead.
Podman network issue
This was triggered when the internal backup container is created. Because it was trying to use the main backup containers network, without a pod being specified, and not started as part of the same compose file.